Merge 'release/6.6' into 'develop' #507
Conversation
Winforms settings.settings version
Update workflow to match release strategy
FantasyTeddy
force-pushed
the
merge-release-branch
branch
from
d2f5d84 to
898dff1
Compare
|
|
||
| namespace AgIO | ||
| { | ||
| public static class Log |
There was a problem hiding this comment.
You dropped the prefix 'C' from the class name. That's perfectly fine for me since it does not add anything in my opinion.
Please change the file name accordingly.
SourceCode/GPS/Classes/CABCurve.cs
Outdated
| @@ -516,14 +516,14 @@ public void GetCurrentCurveLine(vec3 pivot, vec3 steer) | |||
| else// Pure Pursuit ------------------------------------------ | |||
| { | |||
|
|
|||
| double minDistA = double.MaxValue; | |||
| double minDistB = double.MaxValue; | |||
| double minDistA; | |||
There was a problem hiding this comment.
The scope of these variables is too big. Move them inside the if.
SourceCode/GPS/Classes/CLog.cs
Outdated
| { | ||
| StringBuilder sbF = new StringBuilder(); | ||
| long bytes = txtfile.Length - sizeLimit; | ||
| bytes = (sizeLimit *2)/10 + bytes; |
There was a problem hiding this comment.
Use spaces around operators
There was a problem hiding this comment.
Sorry, I don't undestand what the calculation is about. Please add comment line or improve naming of variables if that helps.
SourceCode/GPS/Classes/CLog.cs
Outdated
| { | ||
| bytesSoFar += reader.ReadLine().Length; | ||
| if (bytesSoFar > bytes) | ||
| break; |
There was a problem hiding this comment.
Many coding standards forbid single line if statements like this. Either use {} or put the break on the same line as the if.
There was a problem hiding this comment.
This is the only comment I slightly disagree with: Yes, using brackets ({}) is probably the best way to format this, but if single-line if statements can be used, I don't think it matters if the same-line or the 2-line version is used (they are equally good/bad).
| @@ -461,7 +460,7 @@ private void btnBuildFields_Click(object sender, EventArgs e) | |||
| } | |||
| catch (Exception ed) | |||
| { | |||
| mf.LogEventWriter("Creating new iso field " + ed.ToString()); | |||
| Log.EventWriter("Creating new iso field " + ed.ToString()); | |||
|
|
|||
| MessageBox.Show(gStr.gsError, ed.ToString()); | |||
There was a problem hiding this comment.
It seems that most occurences of Messageboxes for errors also need a Log.EventWriter. In the future we could make an dedicated ErrorMessageBox that takes care of both the eventLog and the window.
SourceCode/GPS/Forms/FormGPS.cs
Outdated
| } | ||
|
|
||
| //get the fields directory, if not exist, create | ||
| logsDirectory = Path.Combine(baseDirectory, "Logs"); | ||
| if (!string.IsNullOrEmpty(logsDirectory) && !Directory.Exists(logsDirectory)) { Directory.CreateDirectory(logsDirectory); | ||
| sbSystemEvents.Append("Logs Dir Created\r"); | ||
| Log.sbEvents.Append("Logs Dir Created\r"); |
There was a problem hiding this comment.
I see so many \r in the calling code. Maybe we could make that the resposibility of the logging class in the future
|
Sorry, these are all valid inputs, but this PR is only the merge of the release branch ( Maybe we can add your proposed changes in a different PR directly to |
SourceCode/GPS/Forms/FormGPS.cs
Outdated
| FileInfo txtfile = new FileInfo(Path.Combine(logsDirectory, "zSystemEventsLog_log.txt")); | ||
| if (txtfile.Exists) | ||
| { | ||
| if (txtfile.Length > (500000)) // ## NOTE: 0.5MB max file size |
There was a problem hiding this comment.
No need for the additional () around 500000. I would prefer to write (512 * 1024), because that is the acutal number for 0.5MB and you don't need to count the zero's when checking the code.
Yes, you are right, this PR is only about the merge. Please ignore my comments. |
I totally understand and I really appreciate the comments. Maybe @farmerbriantee can incorporate these suggestions in a future PR? |
No description provided.